-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
feat(mongodb): Add mongodb client metadata #9930
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: alpha
Are you sure you want to change the base?
Conversation
|
I will reformat the title to use the proper commit message syntax. |
|
🚀 Thanks for opening this pull request! |
📝 WalkthroughWalkthroughVersion metadata is appended to MongoDB clients during connection initialization in two adapter modules. Each adapter adds version information from package.json with a distinct metadata identifier during client setup, without modifying core connection or error-handling logic. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
Could you briefly describe the purpose? |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## alpha #9930 +/- ##
==========================================
- Coverage 93.08% 86.51% -6.57%
==========================================
Files 187 187
Lines 15275 15279 +4
Branches 177 177
==========================================
- Hits 14219 13219 -1000
- Misses 1044 2043 +999
- Partials 12 17 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Adding metadata adds overhead and increase data transfer costs. Also, it's impossible to distinguish multiple servers as these IDs are static. To address both, the metadata should be a Parse Server variable and only if the variable is set should the meta data be set for the driver. Also, is it possible to add a test? |
Sure I'll do that. We basically want handshakes sent to the server to contain metadata about the app that is using the driver so it appears in the server logs when the handshake completes. I'll add the variable and tests. |
|
@mtrezza this is effectively the same change as meteor/meteor#13222. The main purpose is to help differentiate client connections in MongoDB's mongos/mongod logs, so that anyone reviewing their cluster's logs can trace connections more effectively back to the client that created it. The added overhead here would be a handful of bytes, as some metadata is already being sent - this just adds a couple dozen additional bytes to that. |
|
@alexbevi Someones use case should not have a mandatory cost implication for all others. We don't anticipate every possible use case to quantify the implication, nor do we guess someones resource pricing model, nor do we set an arbitrary pain limit for said implication. For the same reason we strive for versatility. Allowing developers to set a custom value via an optional option caters to more use cases, e.g. identify server version, deployment, commit hash, etc. in MongoDB logs. |
I think that's reasonable to make additional information transmission configurable. Would a simple default of the adapter name only (ex: |
|
Opt-in makes more sense to me for the reasons I mentioned. I can't think of a reason to make this opt-out and use a default value. This is an optional log enhancement which is unnecessary in architectures where Parse Server is the only DB client, which I think is the majority, and likely unwanted in production deployments due to the unnecessary overhead. Data transfer can be a significant cost factor in MongoDB Atlas for example, so when thinking about scale, I lean towards opt-in. Even if just a few bytes, think of a scenario where minPoolSize and maxIdleTimeMS repeatedly keeps closing and opening connections, transmitting the additional log data. We strive for more efficiency while giving developers the option to be more verbose in logs, hence I think this PR with opt-in is a good suggestion. Also, enriching the logs with server identifier and version by default exposes information that may provide an additional attack surface for lateral movements. The server version is currently only accessible with the master key. |
Pull Request
The PR incorporates MongoDB's wrapping client library specification for the connection handshake to allow library details to be included in the metadata written to mongos or mongod logs.
Approach
Appends client metadata when mongo clients are created.
Tasks
Summary by CodeRabbit